Skip to content

Align hover label text content with *hoverlabel.align* #3753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 9, 2019
Merged

Align hover label text content with *hoverlabel.align* #3753

merged 4 commits into from
Apr 9, 2019

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Apr 9, 2019

resolves #3726 by adding a new attribute under the hoverlabel trace and layout containers.

See demo: https://codepen.io/etpinard/pen/MRJvLr?editors=1010

I picked align as its behavior reminded me of align in the annotations attributes:

plotly.js/src/components/annotations/attributes.js

Lines 87 to 99 in 1dcee93

align: {
valType: 'enumerated',
values: ['left', 'center', 'right'],
dflt: 'center',
role: 'style',
editType: 'arraydraw',
description: [
'Sets the horizontal alignment of the `text` within the box.',
'Has an effect only if `text` spans more two or more lines',
'(i.e. `text` contains one or more <br> HTML tags) or if an',
'explicit width is set to override the text width.'
].join(' ')
},

Let me know if you can think of a better name.

cc @plotly/plotly_js @nicolaskruchten

etpinard added 4 commits April 9, 2019 10:40
@etpinard
DRY up trace hoverlabel attrs
f20ef2c 
... using their layout hoverlabel counterparts
@etpinard
add hoverlabel.align
6cd54ae 
- with values 'left', 'right' and 'auto' (the dflt and current behavior)
@etpinard
implement hoverlabel.align to loneHover traces
7983568
@etpinard
more tolerant tests for ci
307287e
@etpinard etpinard added this to the v1.47.0 milestone Apr 9, 2019
@nicolaskruchten
Copy link
Contributor

The default is center? I would have thought the default would be “auto” to describe the current behaviour

@nicolaskruchten
Copy link
Contributor

Also re the description: this doesn’t just apply to “texts right? It applies to the entire contents of the hoverlabel, even if it’s built with hovertemplate etc?

@antoinerg
Copy link
Contributor

antoinerg commented Apr 9, 2019 •
edited
Loading

Also re the description: this doesn’t just apply to “texts right? It applies to the entire contents of the hoverlabel, even if it’s built with hovertemplate etc?

hovertemplate was implemented so that it reuses all the existing logic. So yes the hoverlabel.align does apply to the entire content of the hoverlabel even if it's built with hovertemplate!

I think we could replace `text` by text in the description.

The default is center? I would have thought the default would be “auto” to describe the current behaviour

The default is actually auto. However, center is not yet implemented in this PR. @nicolaskruchten is this a requirement?

@etpinard
Copy link
Contributor Author

etpinard commented Apr 9, 2019

The default is center? I would have thought the default would be “auto” to describe the current behaviou

The default is 'auto'.

@etpinard
Copy link
Contributor Author

etpinard commented Apr 9, 2019

Wow. Sorry I made things confusing. The block I pasted in the PR description is in annotations/attributes.js not the new hoverlabel.align attribute declaration which is:

        align: {
            valType: 'enumerated',
            values: ['left', 'right', 'auto'],
            dflt: 'auto',
            role: 'style',
            editType: 'none',
            description: [
                'Sets the horizontal alignment of the text content within hover label box.',
                'Has an effect only if the hover label text spans more two or more lines'
            ].join(' ')
        },

as found in fx/layout_attributes.js.

@antoinerg
Copy link
Contributor

@etpinard about your #3726 (comment) . Since there are two different things we may want to align, wouldn't it be safer to call this new attribute textalign instead of align. I could see align being used to align with respect to the data points as you pointed out whereas in the current PR we deal with text alignment.

@etpinard
Copy link
Contributor Author

etpinard commented Apr 9, 2019

Since there are two different things we may want to align, wouldn't it be safer to call this new attribute textalign instead of align

Fair. But you could say the same thing about annotations.

@nicolaskruchten
Copy link
Contributor

center is not important to me BTW :)

@etpinard
Copy link
Contributor Author

etpinard commented Apr 9, 2019

center is not important to me BTW :)

🎉 that's what I thought.

@nicolaskruchten
Copy link
Contributor

✨ from me :)

@antoinerg
Copy link
Contributor

💃 once the tests pass!

@etpinard
Copy link
Contributor Author

etpinard commented Apr 9, 2019

Going with hoverlabel.align for consistency with annotations.

@etpinard etpinard merged commit 78e7cc6 into master Apr 9, 2019
@etpinard etpinard deleted the align-hover-text branch April 9, 2019 17:55
@etpinard etpinard mentioned this pull request Apr 9, 2019
Closed
@etpinard etpinard mentioned this pull request Apr 17, 2019
Merged
@etpinard etpinard mentioned this pull request Apr 24, 2019
Closed
@nicolaromano
Copy link

Would it be possible to allow center-aligning text as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature something new
Projects
None yet
Milestone
v1.47.0
Development

Successfully merging this pull request may close these issues.

Hoverlabel contents alignment
4 participants
@etpinard @nicolaskruchten @antoinerg @nicolaromano